Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] Massive Bug Fixes / Массовое Исправление Багов #131

Merged
merged 12 commits into from
Dec 1, 2024

Conversation

Spatison
Copy link
Member

@Spatison Spatison commented Nov 29, 2024

Описание PR

Исправлено много ошибок из канала с багрепортами.
Швабра теперь моет весь тайл.
Убрал Из прототипа ДрайДока боксера и психолого, чтобы они не срали в тесты из-за отсутствия их спавна.
Изменения прототипа следов взято с: space-syndicate/space-station-14-next#67


Изменения

🆑 Spatison

  • tweak: Швабра теперь моет весь тайл
  • tweak: Увеличено количество чистящего вещества в чистящих гранатах с 30 до 50
  • tweak: ЕГан теперь имеет размер 4x2 и вешается на пояс
  • tweak: Автоматическое завершение смены снова убрано
  • tweak: Следы теперь полноценные лужи
  • tweak: Шахтерский бур теперь не тратит стамину и работает быстрее
  • fix: Открытие дверей с открытой крышкой теперь работает корректно
  • fix: Интерфейс лобби теперь подстраивается под разрешение экрана правильно
  • fix: СЛР теперь работает в не закрывающих лицо масках
  • fix: Кусачки для растений теперь наносят урон
  • fix: Погода теперь работает корректно и не кладет клиент
  • fix: Хранилища теперь работают корректно

Spatison and others added 7 commits November 26, 2024 20:19
<!--
This is a semi-strict format, you can add/remove sections as needed but
the order/format should be kept the same
Remove these comments before submitting
-->

# Description

<!--
Explain this PR in as much detail as applicable

Some example prompts to consider:
How might this affect the game? The codebase?
What might be some alternatives to this?
How/Who does this benefit/hurt [the game/codebase]?
-->

Fixes the bug with opening storage containers while there's already one
opened.

---

# Changelog

<!--
You can add an author after the `:cl:` to change the name that appears
in the changelog (ex: `:cl: Death`)
Leaving it blank will default to your GitHub display name
This includes all available types for the changelog
-->

:cl:
- fix: Fixed the bug with opening storage containers while there's
already one open.

(cherry picked from commit b0407604ced2859ccbdc417345dadc29656f71f0)
(cherry picked from commit 9a40c3783eb0d0e622badc2febcaf0794f6f8ddd)
@Spatison Spatison self-assigned this Nov 29, 2024
Copy link
Contributor

coderabbitai bot commented Nov 29, 2024

Walkthrough

The pull request introduces several changes across multiple files, focusing on UI logic, system refactoring, and entity definitions. Key modifications include updates to visibility logic in the LobbyGui, the deprecation of a property in StorageBoundUserInterface, enhancements to audio management in WeatherSystem, and various refactorings involving shared components in nutrition and event handling systems. Additionally, new entities and modifications to existing ones are introduced in the prototypes, enhancing gameplay mechanics and interactions.

Changes

File Change Summary
Content.Client/Lobby/UI/LobbyGui.xaml.cs Modified visibility logic for RightSide in SwitchState method; adjusted condition threshold from 0.30 to 0.15. Commented-out DiscordButton functionality.
Content.Client/Storage/StorageBoundUserInterface.cs Marked DeferredClose property as obsolete, returning false.
Content.Client/Weather/WeatherSystem.cs Updated audio handling in Run method; modified SetState method for audio stream management; adjusted gain logic.
Content.IntegrationTests/Tests/Fluids/AbsorbentTest.cs Updated test methods to change parameters for Mop method; instantiated transform variable from SharedTransformSystem.
Content.Server/Animals/Systems/WoolySystem.cs Changed nutrition import from Content.Server.Nutrition to Content.Shared.Nutrition.
Content.Server/Doors/Systems/AirlockSystem.cs Altered event subscription order in Initialize method; added check in OnActivate method for args.Handled.
Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs Added new dependencies; updated Mop and TryPuddleInteract method signatures; introduced AbsorptionRange constant.
Content.Server/FootPrint/FootPrintsSystem.cs Added dependency on StandingStateSystem; removed query for LayingDownComponent.
Content.Server/FootPrint/PuddleFootPrintsSystem.cs Updated OnStepTrigger method to include checks for SolutionContainerManagerComponent.
Content.Server/Polymorph/Systems/PolymorphSystem.cs Changed nutrition import from Content.Server.Nutrition to Content.Shared.Nutrition.
Content.Server/Vampire/BloodSuckerSystem.cs Replaced nutrition component import; updated blood-sucking logic.
Content.Shared/CCVar/CCVars.cs Updated default value of EmergencyShuttleAutoCallTime from 120 to 0.
Content.Shared/Medical/CPR/Systems/CPRSystem.cs Added dependency on IGameTiming; updated popup handling in StartCPR method.
Content.Shared/Nutrition/Components/IngestionBlockerComponent.cs Moved to Content.Shared.Nutrition.Components; modified component registration attributes.
Content.Shared/Nutrition/EntitySystems/IngestionBlockerSystem.cs Transitioned to Content.Shared.Nutrition.EntitySystems; updated using directives.
Content.Shared/Nutrition/IngestionEvents.cs Changed namespace from Content.Server.Nutrition to Content.Shared.Nutrition.
Content.Shared/Prying/Systems/PryingSystem.cs Introduced time calculation for prying actions; updated StartPry method.
Resources/Changelog/Changelog.yml Added new features, traits, items, and fixed various gameplay bugs.
Resources/Prototypes/DeltaV/Entities/Objects/Weapons/Guns/Battery/battery_guns.yml Enhanced definitions of energy gun entities; introduced new variants.
Resources/Prototypes/Entities/Effects/puddle.yml Created new abstract entity PuddleTemporary and updated existing puddle entities.
Resources/Prototypes/Entities/Objects/Specific/Hydroponics/tools.yml Updated damage type and added new components for hydroponics tools.
Resources/Prototypes/Entities/Objects/Weapons/Throwable/grenades.yml Added new grenade types and modified existing entities.
Resources/Prototypes/Maps/drydock.yml Removed job entries for "Psychologist" and "Boxer".

Possibly related PRs

  • DryDock update #68: The changes in the main PR regarding the visibility logic in LobbyGui.xaml.cs may relate to the updates in the drydock.yml file, which includes job roles and player counts that could affect UI elements in the lobby interface.
  • UpdateDryDockV15 #98: The modifications in the drydock.yml file about job roles and player counts could also impact the UI logic in the main PR, as these roles may be displayed in the lobby interface.
  • [Tweak] Operative UI Adaptation #128: The UI adaptations in the Operative UI Adaptation PR could be relevant to the changes in the visibility logic of UI elements in the main PR, as both involve enhancements to user interface elements.

Suggested labels

Status: Needs Review

Suggested reviewers

  • Remuchi
  • DVOniksWyvern

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (25)
Content.Shared/Prying/Systems/PryingSystem.cs (2)

Line range hint 131-134: Prevent potential division by zero in time calculation

The calculation of time divides by toolModifier, which could potentially be zero if not properly validated. This could lead to a division by zero exception at runtime. Ensure that toolModifier is checked to be non-zero before performing the division.

Apply this diff to add a validation check:

 var time = modEv.BaseTime * modEv.PryTimeModifier / toolModifier;

+if (toolModifier == 0)
+{
+    throw new InvalidOperationException("Tool modifier cannot be zero.");
+}

 if (time <= modEv.Neglect)
     time = 0;

Line range hint 179-179: Clarify condition logic to ensure correct evaluation

The condition door.State is not DoorState.Closing and not DoorState.Opening may not evaluate as intended due to operator precedence. Without parentheses, the logic could be misinterpreted, leading to unintended behavior. Consider adding parentheses or refactoring the condition for clarity.

Apply this diff to clarify the condition:

-if (args.Used != null && comp != null && door.State is not DoorState.Closing and not DoorState.Opening)
+if (args.Used != null && comp != null && door.State is not (DoorState.Closing or DoorState.Opening))

Alternatively, refactor the condition:

-if (args.Used != null && comp != null && door.State is not DoorState.Closing and not DoorState.Opening)
+if (args.Used != null && comp != null && door.State != DoorState.Closing && door.State != DoorState.Opening)
Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs (3)

313-317: Re-evaluate the use of break in the puddle interaction loop

When available == FixedPoint2.Zero, the code pops up a message and then uses break to exit the foreach loop. This means that if the absorber lacks the necessary solution, it will not attempt to interact with any remaining puddles in the targets collection. Confirm that this early exit is intentional and won't prevent interactions with other puddles that might still be valid.

Consider handling this condition per puddle without exiting the loop prematurely, allowing the absorber to interact with other puddles if possible.


344-347: Add comments to clarify position transformations for the lunge action

The calculations for localPos involve complex transformations, including inverses and rotations. Adding explanatory comments can improve code readability and help future maintainers understand the purpose and necessity of these calculations.


285-289: Improve error handling for grid and tile retrieval

When attempting to find the grid and tile at the specified coordinates, there's a possibility that the grid might not exist, or the coordinates could be invalid. Currently, the code returns false if _mapManager.TryFindGridAt fails. Consider logging an error or providing feedback to the user to aid in debugging potential issues with coordinate validity.

Content.Server/Animals/Systems/WoolySystem.cs (2)

12-14: Consider enhancing the documentation.

While the current documentation is accurate, it could be more comprehensive by:

  • Explaining the hunger mechanics and its impact on production
  • Documenting the HungerComponent interaction
  • Adding parameter ranges for production rates
 /// <summary>
-///     Gives ability to produce fiber reagents, produces endless if the
-///     owner has no HungerComponent
+///     Handles the production of fiber reagents by wooly entities.
+///     Production is governed by hunger levels when a HungerComponent is present,
+///     requiring an "Okay" hunger threshold. Without HungerComponent, production is unlimited.
+///     Production occurs at regular intervals defined by GrowthDelay.
 /// </summary>

Line range hint 31-53: Consider adding safety measures and logging.

The wool production logic could benefit from additional safeguards:

  1. Consider implementing a production limit even without HungerComponent
  2. Add solution container capacity checks
  3. Include error logging for failed operations
 while (query.MoveNext(out var uid, out var wooly))
 {
     if (now < wooly.NextGrowth)
         continue;

     wooly.NextGrowth = now + wooly.GrowthDelay;

     if (_mobState.IsDead(uid))
         continue;

+    // Track production count even without hunger
+    if (!EntityManager.TryGetComponent(uid, out HungerComponent? hunger))
+    {
+        var maxProductionPerDay = 100; // Configure as needed
+        if (wooly.DailyProduction >= maxProductionPerDay)
+            continue;
+        wooly.DailyProduction++;
+    }
+
     // Actually there is food digestion so no problem with instant reagent generation "OnFeed"
-    if (EntityManager.TryGetComponent(uid, out HungerComponent? hunger))
+    else
     {
         // Is there enough nutrition to produce reagent?
         if (_hunger.GetHungerThreshold(hunger) < HungerThreshold.Okay)
             continue;

         _hunger.ModifyHunger(uid, -wooly.HungerUsage, hunger);
     }

     if (!_solutionContainer.ResolveSolution(uid, wooly.SolutionName, ref wooly.Solution))
+    {
+        Logger.Warning($"Failed to resolve solution {wooly.SolutionName} for wooly entity {uid}");
         continue;
+    }

+    if (!_solutionContainer.CanAddReagent(wooly.Solution.Value, wooly.ReagentId, wooly.Quantity))
+    {
+        Logger.Debug($"Solution container full for wooly entity {uid}");
+        continue;
+    }

     _solutionContainer.TryAddReagent(wooly.Solution.Value, wooly.ReagentId, wooly.Quantity, out _);
 }
Content.Server/FootPrint/PuddleFootPrintsSystem.cs (4)

Line range hint 33-34: Add null check and consider using a constant for "Water"

The water quantity calculation could be improved:

  1. Consider using a constant instead of the hardcoded "Water" string
  2. The LINQ query could be simplified
-var waterQuantity = (from sol in solutions.Contents where sol.Reagent.Prototype == "Water" select (float) sol.Quantity).FirstOrDefault();
+private const string WaterPrototype = "Water";
+var waterQuantity = solutions.Contents
+    .Where(sol => sol.Reagent.Prototype == WaterPrototype)
+    .Select(sol => (float)sol.Quantity)
+    .FirstOrDefault();

Line range hint 36-36: Prevent potential division by zero

The calculation totalSolutionQuantity / 100f could throw if totalSolutionQuantity is 0.

-if (waterQuantity / (totalSolutionQuantity / 100f) > component.OffPercent || solutions.Contents.Count <= 0)
+if (totalSolutionQuantity == 0 || solutions.Contents.Count <= 0 || waterQuantity / (totalSolutionQuantity / 100f) > component.OffPercent)

Line range hint 39-40: Simplify highest quantity reagent selection

The LINQ aggregate operation could be simplified using MaxBy.

-tripper.ReagentToTransfer =
-    solutions.Contents.Aggregate((l, r) => l.Quantity > r.Quantity ? l : r).Reagent.Prototype;
+tripper.ReagentToTransfer = solutions.Contents
+    .MaxBy(x => x.Quantity)?.Reagent.Prototype;

Line range hint 14-48: Consider splitting solution management logic

The PuddleFootPrintsSystem handles both footprint mechanics and solution management. Consider extracting the solution-related logic into a separate service to improve separation of concerns and maintainability.

Content.Server/Doors/Systems/AirlockSystem.cs (2)

24-24: LGTM: Event handling order change improves system interaction.

The modification ensures that DoorSystem handles activation events before AirlockSystem, which is the correct hierarchical order since airlocks are a specialized type of door.

This change follows the principle of processing events from base to specialized systems, which helps prevent conflicts in door functionality.


70-74: LGTM: Added proper event handling check.

The early return when the event is already handled is a good defensive programming practice that complements the event order change.

Consider adding a debug log when skipping already handled events to help with troubleshooting:

  if (args.Handled)
+     Logger.Debug($"Skipping airlock activation for {ToPrettyString(uid)} - event already handled");
      return;
Content.Server/FootPrint/FootPrintsSystem.cs (2)

26-26: Remove non-standard comment marker

While the dependency injection is good, let's remove the "WD EDIT" comment as it doesn't follow standard documentation practices.

-    [Dependency] private readonly StandingStateSystem _standingState = default!; // WD EDIT
+    [Dependency] private readonly StandingStateSystem _standingState = default!;

Line range hint 1-124: Architecture aligns well with PR objectives

The changes to FootPrintsSystem effectively support the PR objective of making footprints appear as full puddles while improving the state management architecture. The system properly handles:

  • Different character states (standing/down) through StandingStateSystem
  • Fluid interactions through solution containers
  • Various footprint appearances based on equipment and state

Consider adding unit tests to verify the interaction between StandingStateSystem and footprint generation, especially for edge cases in state transitions.

Content.Client/Weather/WeatherSystem.cs (2)

120-120: LGTM with a minor typo fix.

The optimization to reuse the already validated AudioComponent is good. However, there's a typo in the comment ("EDIR" should be "EDIT").

-        _audio.SetGain(weather.Stream, alpha, comp); // WD EDIR
+        _audio.SetGain(weather.Stream, alpha, comp); // WD EDIT

Line range hint 1-165: Consider enhancing audio resource cleanup.

While the changes improve audio handling, consider implementing IDisposable or adding explicit cleanup in OnRemove to ensure audio resources are properly disposed when the weather system is shut down.

Example implementation:

-public sealed class WeatherSystem : SharedWeatherSystem
+public sealed class WeatherSystem : SharedWeatherSystem, IDisposable
 {
     // ... existing code ...
+
+    public void Dispose()
+    {
+        foreach (var (_, weather) in GetWeathers())
+        {
+            weather.Stream = _audio.Stop(weather.Stream);
+        }
+    }
 }
Content.Shared/Medical/CPR/Systems/CPRSystem.cs (3)

32-32: Consider removing or standardizing edit markers

The "WD EDIT" comment might not follow project conventions. Consider either removing it or using a more standard format for tracking changes.

-        [Dependency] private readonly IGameTiming _timing = default!; // WD EDIT
+        [Dependency] private readonly IGameTiming _timing = default!;

63-66: LGTM: Prediction check properly implemented

The prediction check is correctly placed to prevent duplicate processing. However, consider removing the "WD EDIT" markers for consistency.

-            // WD EDIT START
-            if (!_timing.IsFirstTimePredicted)
-                return;
-            // WD EDIT END
+            if (!_timing.IsFirstTimePredicted)
+                return;

70-93: LGTM: Popup and mask checks improved, with potential for refactoring

The changes correctly implement predicted popups and mask blocking checks. However, there's an opportunity to reduce code duplication in the mask checking logic.

Consider extracting the mask check logic into a helper method:

+        private bool IsMaskBlocking(EntityUid? maskEntity)
+        {
+            return maskEntity != null && 
+                   TryComp<IngestionBlockerComponent>(maskEntity, out var blocker) && 
+                   blocker.Enabled;
+        }

         private void StartCPR(EntityUid performer, EntityUid target, CPRTrainingComponent cprComponent)
         {
             // ... existing code ...

-            if (_inventory.TryGetSlotEntity(target, "mask", out var mask)
-                && TryComp<IngestionBlockerComponent>(mask, out var blockerTarget)
-                && blockerTarget.Enabled)
+            if (_inventory.TryGetSlotEntity(target, "mask", out var mask) && IsMaskBlocking(mask))
             {
                 _popupSystem.PopupPredicted(Loc.GetString("cpr-must-remove", ("clothing", mask)), performer, performer, PopupType.MediumCaution);
                 return;
             }

-            if (_inventory.TryGetSlotEntity(performer, "mask", out var maskSelf)
-                && TryComp<IngestionBlockerComponent>(maskSelf, out var blockerPerformer)
-                && blockerPerformer.Enabled)
+            if (_inventory.TryGetSlotEntity(performer, "mask", out var maskSelf) && IsMaskBlocking(maskSelf))
             {
                 _popupSystem.PopupPredicted(Loc.GetString("cpr-must-remove-own-mask", ("clothing", maskSelf)), performer, performer, PopupType.MediumCaution);
                 return;
             }
Resources/Prototypes/DeltaV/Entities/Objects/Weapons/Guns/Battery/battery_guns.yml (1)

Line range hint 150-154: Consider adding visual distinctions for security variants

While the security variants are properly implemented with clear descriptions, consider adding visual distinctions (e.g., different sprites or colors) to help players easily identify security-issued weapons.

Also applies to: 240-244

Resources/Prototypes/Entities/Objects/Weapons/Throwable/grenades.yml (1)

406-406: LGTM! Consider improving the edit comment.

The increase in SpaceCleaner quantity from 30 to 50 aligns well with the PR objective and maintains consistency with similar grenade types. However, the comment "# WD EDIT" could be more descriptive.

Consider replacing the comment with something more descriptive like:

-        Quantity: 50 # WD EDIT
+        Quantity: 50 # Increased from 30 to match cleaning requirements for full tile coverage
Resources/Changelog/Changelog.yml (3)

Line range hint 1-8544: Changelog entries should follow consistent formatting

The changelog contains a mix of formatting styles and languages. Consider standardizing:

  1. All entries should have a URL field
  2. Message formatting should be consistent (some use markdown, others plain text)
  3. Bilingual entries should separate languages with a forward slash consistently
  4. Type field values should be standardized to: Add, Remove, Fix, Tweak

Consider adding validation to enforce consistent formatting:

Entries:
  - author: string # Required
    changes:
      - message: string # Required, use consistent formatting
        type: enum[Add|Remove|Fix|Tweak] # Required, standardized types
    id: integer # Required
    time: datetime # Required ISO8601
    url: url # Required, valid URL

8477-8478: Avoid generic changelog messages

The message "Upstream :D" is not descriptive enough to be useful. Changelog entries should clearly describe what changed.

Consider replacing with a more descriptive message that explains what upstream changes were integrated.


Line range hint 8484-8485: Use specific descriptions for bug fixes

The message "Массовый баг фикс" (Mass bug fix) is too vague. Bug fix entries should describe what specific issues were fixed.

Consider listing the key bugs that were fixed to help users understand the impact.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 762c491 and 35a0dc7.

📒 Files selected for processing (23)
  • Content.Client/Lobby/UI/LobbyGui.xaml.cs (1 hunks)
  • Content.Client/Storage/StorageBoundUserInterface.cs (1 hunks)
  • Content.Client/Weather/WeatherSystem.cs (2 hunks)
  • Content.IntegrationTests/Tests/Fluids/AbsorbentTest.cs (5 hunks)
  • Content.Server/Animals/Systems/WoolySystem.cs (1 hunks)
  • Content.Server/Doors/Systems/AirlockSystem.cs (2 hunks)
  • Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs (5 hunks)
  • Content.Server/FootPrint/FootPrintsSystem.cs (3 hunks)
  • Content.Server/FootPrint/PuddleFootPrintsSystem.cs (1 hunks)
  • Content.Server/Polymorph/Systems/PolymorphSystem.cs (1 hunks)
  • Content.Server/Vampire/BloodSuckerSystem.cs (1 hunks)
  • Content.Shared/CCVar/CCVars.cs (1 hunks)
  • Content.Shared/Medical/CPR/Systems/CPRSystem.cs (3 hunks)
  • Content.Shared/Nutrition/Components/IngestionBlockerComponent.cs (2 hunks)
  • Content.Shared/Nutrition/EntitySystems/IngestionBlockerSystem.cs (1 hunks)
  • Content.Shared/Nutrition/IngestionEvents.cs (1 hunks)
  • Content.Shared/Prying/Systems/PryingSystem.cs (1 hunks)
  • Resources/Changelog/Changelog.yml (2 hunks)
  • Resources/Prototypes/DeltaV/Entities/Objects/Weapons/Guns/Battery/battery_guns.yml (1 hunks)
  • Resources/Prototypes/Entities/Effects/puddle.yml (2 hunks)
  • Resources/Prototypes/Entities/Objects/Specific/Hydroponics/tools.yml (1 hunks)
  • Resources/Prototypes/Entities/Objects/Weapons/Throwable/grenades.yml (1 hunks)
  • Resources/Prototypes/Maps/drydock.yml (0 hunks)
💤 Files with no reviewable changes (1)
  • Resources/Prototypes/Maps/drydock.yml
✅ Files skipped from review due to trivial changes (3)
  • Content.Server/Polymorph/Systems/PolymorphSystem.cs
  • Content.Shared/Nutrition/EntitySystems/IngestionBlockerSystem.cs
  • Content.Shared/Nutrition/IngestionEvents.cs
🔇 Additional comments (30)
Content.Server/Vampire/BloodSuckerSystem.cs (1)

17-17: Verify compatibility with Shared Nutrition Components

Switching the import from Content.Server.Nutrition.Components to Content.Shared.Nutrition.Components may impact how nutrition components function within the server context. Ensure that all references to these components, such as StomachComponent, are compatible with the shared versions and that no server-specific functionality is lost.

Run the following script to check for any remaining references to the old namespace and to verify component usage:

Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs (3)

285-289: Verify coordinate transformations when locating the grid and tile

In TryPuddleInteract, the coordinate transformations involve converting coordinates to map coordinates and adjusting positions with an offset of 0.5f. This offset appears to center the interaction area, but verify that this calculation accurately represents the desired position to prevent any off-by-one errors or misalignments.


Line range hint 373-386: Ensure playSound flag is set appropriately during puddle interactions

The playSound flag is set to true inside the puddle interaction loop when absorption occurs. Ensure that this flag accurately reflects whether any interaction took place. If none of the puddles are interacted with due to conditions like insufficient absorber volume or puddle properties, the sound should not play. Confirm that the logic correctly represents this behavior.


Line range hint 115-132: Ensure all calls to Mop are updated to match the new signature

The Mop method signature has been updated to accept EntityCoordinates coordinates and an optional EntityUid? target. Ensure that all calls to this method within the codebase are updated accordingly to prevent any runtime issues due to mismatched parameters.

Run the following script to identify any outdated calls to Mop:

✅ Verification successful

Based on the search results, I can see that there are two calls to the Mop method in AbsorbentTest.cs, and both calls are already using the correct signature with EntityCoordinates parameter. Let me verify if there are any other potential calls using AST to ensure we haven't missed anything.


All calls to Mop are already using the correct signature

All four calls to the Mop method across the codebase have been verified:

  • Two calls in AbsorbentTest.cs using transform.GetMoverCoordinates(absorbent)
  • Two calls in AbsorbentSystem.cs using args.ClickLocation and args.Target

All calls are already passing the required EntityCoordinates parameter and optional EntityUid? target, matching the new method signature. No updates are needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all usages of the `Mop` method that may need updating.

# Search for method calls to `Mop` that do not match the new signature
rg --glob '!AbsorbentSystem.cs' -t cs -A 2 'Mop\('

Length of output: 639


Script:

#!/bin/bash
# Use ast-grep to find method invocations of Mop
ast-grep --pattern 'Mop($$$)'

Length of output: 295

Content.Shared/Nutrition/Components/IngestionBlockerComponent.cs (3)

Line range hint 11-19: LGTM! Clean and well-documented implementation.

The component implementation is focused, properly documented, and follows best practices:

  • Clear XML documentation explaining the purpose and potential future considerations
  • Proper use of ViewVariables and DataField attributes
  • Simple boolean state management

1-1: Verify the necessity of client-side access.

The component has been moved to the Shared namespace, implying it's needed on both client and server. This aligns with the PR objective regarding SLR and masks, but we should verify if client-side systems actually need this component.

✅ Verification successful

Let me check the actual usage in these systems to better understand if the shared namespace is justified.


Let me check the server-side systems to understand their interaction with this component and ensure the shared implementation is appropriate.


Moving to Shared namespace is justified

The component's move to the Shared namespace is appropriate because:

  1. It's used by the CPR system which needs client-side prediction for popups
  2. The IngestionBlockerSystem handles mask toggle events which need to be synchronized
  3. Server-side systems (FoodSystem, BloodSuckerSystem, FelinidSystem) only read the component state, making it safe for shared access
  4. The component's simple boolean state and mask-toggle logic are suitable for client-side prediction
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any client-side systems that reference this component

# Search for client-side systems using this component
echo "Checking Client-side usage:"
rg -l "IngestionBlockerComponent" Content.Client/

# Search for shared systems using this component
echo -e "\nChecking Shared systems usage:"
rg -l "IngestionBlockerComponent" Content.Shared/

Length of output: 419


Script:

#!/bin/bash
# Check the implementation details of systems using this component
echo "=== Shared IngestionBlockerSystem implementation ==="
rg "class IngestionBlockerSystem" -A 10 Content.Shared/Nutrition/EntitySystems/IngestionBlockerSystem.cs

echo -e "\n=== CPRSystem usage ==="
rg "IngestionBlocker" -A 5 Content.Shared/Medical/CPR/Systems/CPRSystem.cs

# Check if there are any server-side systems still using this
echo -e "\n=== Server-side usage ==="
rg -l "IngestionBlockerComponent" Content.Server/

Length of output: 1705


Script:

#!/bin/bash
# Check how server systems use this component
echo "=== FoodSystem usage ==="
rg "IngestionBlocker" -A 5 Content.Server/Nutrition/EntitySystems/FoodSystem.cs

echo -e "\n=== BloodSuckerSystem usage ==="
rg "IngestionBlocker" -A 5 Content.Server/Vampire/BloodSuckerSystem.cs

echo -e "\n=== FelinidSystem usage ==="
rg "IngestionBlocker" -A 5 Content.Server/Nyanotrasen/Abilities/Felinid/FelinidSystem.cs

Length of output: 1576


10-10: Review the removal of access restrictions.

The removal of Access attributes (typeof(FoodSystem), typeof(DrinkSystem), typeof(IngestionBlockerSystem)) makes this component accessible to all systems. While this enables more flexible integration, it could potentially lead to misuse.

Content.Client/Storage/StorageBoundUserInterface.cs (1)

12-12: 🛠️ Refactor suggestion

Add an obsolete message explaining the deprecation.

The Obsolete attribute should include a message explaining why the property is deprecated and what developers should use instead. This helps with future maintenance and migration.

Apply this diff:

-    [Obsolete] public override bool DeferredClose => false;
+    [Obsolete("DeferredClose is no longer used for storage UI. Use StorageSystem.CloseStorageWindow instead.")] 
+    public override bool DeferredClose => false;

Additionally, let's verify the impact of hardcoding this property to false:

✅ Verification successful

Let me gather more information about the base class and storage system to better understand the context.


Let me try one more search to find the base class definition and any other UI classes that might be using DeferredClose.


Based on the search results, I can now generate the final response. The codebase shows that DeferredClose is not used in any meaningful way in the storage system, and the new StorageSystem.CloseStorageWindow is the correct method being used for closing storage windows.

Add an obsolete message and ensure correct deprecation.

The DeferredClose property is no longer used for storage UI handling, and StorageSystem.CloseStorageWindow is the established method for closing storage windows. The property can be safely marked as obsolete with an explanatory message.

-    [Obsolete] public override bool DeferredClose => false;
+    [Obsolete("DeferredClose is no longer used for storage UI. Use StorageSystem.CloseStorageWindow instead.")] 
+    public override bool DeferredClose => false;
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for references to DeferredClose to understand its usage
# and potential impact of hardcoding to false

# Search for any references to DeferredClose in the codebase
echo "Searching for DeferredClose references:"
rg "DeferredClose" -A 3

# Search for the base class to understand the original behavior
echo -e "\nSearching for the base class definition:"
ast-grep --pattern 'class $_ {
  $$$
  DeferredClose
  $$$
}'

Length of output: 674


Script:

#!/bin/bash
# Search for StorageSystem.CloseStorageWindow to verify the suggested alternative
echo "Searching for StorageSystem.CloseStorageWindow:"
rg "CloseStorageWindow" -A 3

# Search for BoundUserInterface class to understand the base implementation
echo -e "\nSearching for BoundUserInterface class:"
fd "BoundUserInterface.cs" --exec rg -A 5 "class BoundUserInterface"

# Search for any recent changes or removals related to DeferredClose
echo -e "\nSearching for any recent changes mentioning DeferredClose:"
git log -p --grep="DeferredClose" --max-count=2

Length of output: 1911


Script:

#!/bin/bash
# Search for any class that has DeferredClose property
echo "Searching for classes with DeferredClose property:"
ast-grep --pattern 'class $_ {
  $$$
  public $_ bool DeferredClose
  $$$
}'

# Search for any interface definitions that might declare DeferredClose
echo -e "\nSearching for interface with DeferredClose:"
ast-grep --pattern 'interface $_ {
  $$$
  bool DeferredClose
  $$$
}'

# Search for files containing "BoundUserInterface" to find the base class
echo -e "\nSearching for BoundUserInterface references:"
rg "class.*BoundUserInterface" -A 3

Length of output: 101751

Content.Server/Animals/Systems/WoolySystem.cs (1)

4-6: Verify the nutrition component migration impact.

The switch from server-specific to shared nutrition components looks good, but let's ensure all references are updated correctly.

Content.Server/FootPrint/PuddleFootPrintsSystem.cs (2)

2-2: LGTM: New imports are properly organized and necessary

The added imports support the enhanced fluid management and mapping functionality.

Also applies to: 8-8


Line range hint 43-46: Verify visual update logic

The visual updates align with the PR objective of making footprints appear as full puddles. However, ensure that the SizeRatio calculation produces the intended visual effect.

✅ Verification successful

Let me search for the SizeRatio field to understand its usage in the codebase.


Let me check the implementation of the footprint system to better understand how SizeRatio affects the visual appearance.


Based on the code analysis, I can now generate the final response:

SizeRatio is correctly used for footprint scaling

The SizeRatio of 0.2f in PuddleFootPrintsComponent is used to scale the visual effect of footprints based on puddle volume. The implementation in PuddleFootPrintsSystem multiplies this ratio with the puddle volume to determine the color intensity of the footprint (volume * component.SizeRatio). This scaling ensures footprints appear proportional to the amount of liquid stepped in, which aligns with the PR's objective of realistic footprint visuals.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other usages of SizeRatio to verify consistency
rg -A 2 "SizeRatio.*=|get;|set;" --type cs

Length of output: 105863


Script:

#!/bin/bash
# Search for SizeRatio field definitions and usages
rg -A 2 "SizeRatio.*[=;]" --type cs

Length of output: 1073


Script:

#!/bin/bash
# Search for footprint-related implementation
rg -A 10 "AddColor.*tripper" --type cs

Length of output: 1068

Content.Client/Lobby/UI/LobbyGui.xaml.cs (2)

49-49: LGTM: Initial visibility state is correct

Initially hiding the right side panel when entering character setup provides a clean slate for the subsequent width-based visibility logic.


57-60: Verify the new threshold for different screen resolutions

The threshold for showing the right side panel has been made more conservative (from 70% to 85% remaining space). While this should help with the reported resolution issues, we should verify it works well across different screen sizes.

Consider adding debug logging to help track these UI layout decisions. For example:

 if (1 - (setupWidth / actualWidth) < 0.15)
+    Logger.Debug($"LobbyGui: Showing right side panel. Setup width: {setupWidth}, Screen width: {actualWidth}, Ratio: {1 - (setupWidth / actualWidth)}");
     RightSide.Visible = true;

This would help diagnose any future resolution-related issues more easily.

Resources/Prototypes/Entities/Objects/Specific/Hydroponics/tools.yml (4)

Line range hint 61-61: LGTM: Sharp component addition to scythe

Adding the Sharp component to the scythe entity is a logical enhancement that aligns with the tool's nature and description.


Line range hint 147-147: LGTM: Shovel component with speed modifier

The addition of the Shovel component with a 0.85 speedModifier is well-justified by the comment explaining it's slower than a full-sized shovel, which makes sense for game balance.


Line range hint 134-134: Review the gameplay implications of spade damage

The comment "I guess you can stab it into them?" suggests potential unintended gameplay mechanics. While the damage values are relatively low (Blunt: 6, Slash: 2), we should consider if this aligns with the game's design philosophy and balance.

Also applies to: 135-135

✅ Verification successful

Spade damage values are within normal range for tools

The spade's damage values (Blunt: 6, Slash: 2) are well-aligned with other tools in the game:

  • Common tools like welders, wrenches, and flashlights have similar Blunt damage (6-6.5)
  • Other gardening tools have comparable mixed damage profiles (e.g. hatchet with Slash: 3.5, Blunt: 3)
  • The values are significantly lower than dedicated weapons like toolboxes (Blunt: 9-12)

The damage is appropriate for a utility tool that could be used defensively but isn't designed as a weapon.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar damage values in other tools for comparison
rg -A 5 'damage:\s*\n\s*types:' | rg -B 1 -A 3 'Blunt|Slash'

Length of output: 254


Script:

#!/bin/bash
# Search for damage configurations in tools and similar items
rg -U --multiline 'damage:[\s\n]+types:' -A 5

Length of output: 139105


Script:

#!/bin/bash
# Search for damage values in tools and similar items, focusing on Blunt and Slash damage types
rg -U --multiline '(Blunt|Slash):\s*\d+(\.\d+)?' Resources/Prototypes/Entities/Objects/Tools/ Resources/Prototypes/Entities/Objects/Specific/Hydroponics/

Length of output: 2818


45-45: Verify the "Piercing" damage type name

The damage type has been changed from "Pierce" to "Piercing". While this change aligns with the PR objective to make plant clippers inflict damage, we should verify that "Piercing" is the correct damage type name used throughout the codebase.

✅ Verification successful

"Piercing" is the correct damage type name

Based on extensive evidence from the codebase:

  • "Piercing" is widely used as a damage type across many entity definitions
  • It's consistently spelled as "Piercing" (not "Pierce") in damage type definitions and usage
  • The damage type is used in various contexts including weapons, armor, and other entities
  • Found in Resources/Prototypes/Damage/types.yml with ID "Piercing"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other occurrences of damage types to verify consistency
rg -A 1 'damage:\s*\n\s*types:' | rg -A 1 'Piercing|Pierce'

Length of output: 253


Script:

#!/bin/bash
# Search for damage type definitions and usage patterns
rg -U "Piercing|Pierce" --type yaml

# Also try searching with ast-grep to find any damage type definitions in code
ast-grep --pattern 'enum DamageType {
  $$$
}'

Length of output: 28382

Resources/Prototypes/Entities/Effects/puddle.yml (2)

203-222: LGTM: Solution and transfer amount changes look good

The changes to the solution container (maxVol: 2) and drink transfer amount (0.5) are well-balanced for footprint mechanics. These values should prevent footprints from becoming too prominent while still maintaining their visibility as intended by the PR objectives.


137-139: Verify collision behavior changes for puddles

The change from SlipLayer to multiple impassable layers (Impassable, MidImpassable, LowImpassable) significantly alters how puddles interact with other entities. This could affect gameplay mechanics.

✅ Verification successful

Collision layer changes for puddles are consistent with similar entities

The change from SlipLayer to multiple impassable layers is consistent with how other ground-level entities handle collisions in the game. Similar entities like ice crusts, shards, and base structures use combinations of these collision layers. The pattern shows:

  • Ground-level hazards (ice, shards) use LowImpassable or MidImpassable
  • Base structures use both MidImpassable and LowImpassable
  • The layering system allows for proper collision handling at different heights
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other entities using these collision layers to understand potential interactions
rg -l "Impassable|MidImpassable|LowImpassable" Resources/Prototypes/Entities/

Length of output: 3487


Script:

#!/bin/bash
# Let's examine the actual usage of these layers in some key files to understand the collision patterns
rg -A 5 "Impassable|MidImpassable|LowImpassable" Resources/Prototypes/Entities/Objects/Misc/ice_crust.yml Resources/Prototypes/Entities/Objects/Materials/shards.yml Resources/Prototypes/Entities/Objects/Specific/Janitorial/janitor.yml

# Also check the base structure definitions for these layers
rg -A 5 "Impassable|MidImpassable|LowImpassable" Resources/Prototypes/Entities/Structures/base_structure.yml

Length of output: 2899

Content.Server/FootPrint/FootPrintsSystem.cs (2)

9-10: LGTM: New imports support puddle functionality

The added imports align with the PR objective of making footprints appear as full puddles.


57-58: Verify standing state coverage

The change from LayingDownComponent to StandingStateSystem looks good, but let's verify that all laying down states are properly handled by IsDown().

✅ Verification successful

Standing state implementation properly handles all laying down cases

The verification shows that IsDown() correctly handles all laying down states by checking for both StandingState.Lying and StandingState.GettingUp states. The implementation is consistent across the codebase, and the migration from LayingDownComponent to StandingStateSystem is complete. The footprint system's use of _standingState.IsDown(uid) correctly captures all cases where an entity is not standing.

Key findings:

  • IsDown() checks for both Lying and GettingUp states
  • Standing state transitions are properly managed through the Down() and Stand() methods
  • The system handles all edge cases including state transitions and crawling mechanics
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all usages of LayingDownComponent to ensure complete migration
ast-grep --pattern 'LayingDownComponent'

# Search for other standing state checks to ensure consistency
ast-grep --pattern $'_standingState.IsDown($_)'

Length of output: 636


Script:

#!/bin/bash
# Let's search for the StandingStateSystem implementation to understand IsDown
ast-grep --pattern $'class StandingStateSystem {
  $$$
}'

# Also search for any references to standing state transitions
rg "StandingState\." -A 3

Length of output: 7388

Content.Client/Weather/WeatherSystem.cs (1)

50-54: LGTM! Defensive programming improvements.

The added null checks and component validation improve the robustness of the audio stream handling, which should help prevent client crashes mentioned in the PR objectives.

Let's verify the audio component handling:

✅ Verification successful

Audio component handling is properly implemented with consistent null checks

The verification shows that all usages of weather.Stream in WeatherSystem.cs are properly guarded:

  • Stream initialization uses null-coalescing (??=) and optional chaining (?.)
  • Component access is protected by TryComp check
  • Stream stopping operations handle null cases implicitly
  • Audio gain setting is only called after the component check
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other potential audio-related crash scenarios in the codebase

# Look for similar patterns where AudioComponent might be used without checks
ast-grep --pattern 'TryComp<AudioComponent>($_, out $_)'

# Look for direct audio stream usage without null checks
rg 'weather\.Stream' -A 2

Length of output: 1227

Content.Shared/Medical/CPR/Systems/CPRSystem.cs (1)

9-9: LGTM: Required imports added correctly

The new imports for Nutrition.Components and Timing are necessary for the implemented changes.

Also applies to: 15-16

Resources/Prototypes/DeltaV/Entities/Objects/Weapons/Guns/Battery/battery_guns.yml (2)

18-23: LGTM: Item size matches PR requirements

The shape definition [0, 0, 3, 1] correctly implements the 4x2 size requirement mentioned in the PR objectives (as coordinates are zero-based, this creates a 4-wide by 2-high grid).


26-30: LGTM: Belt slot added as required

The addition of the Belt slot aligns with the PR objective to make the E-Gun wearable on the belt. The extra suitStorage slot provides additional flexibility.

Content.IntegrationTests/Tests/Fluids/AbsorbentTest.cs (4)

10-10: LGTM!

The added import for Robust.Shared.Physics is necessary for the new coordinate-based mopping functionality.


86-86: LGTM! Test updates align with new mop functionality

The changes properly integrate the new coordinate-based mopping system:

  • Transform system initialization is correctly added
  • Mop method call is updated to use the new signature with coordinates

Also applies to: 115-115


145-145: LGTM! Consistent test updates

The changes mirror those in the first test method, maintaining consistency in the test suite while supporting the new coordinate-based mopping functionality.

Also applies to: 173-173


Line range hint 192-350: Consider adding test cases for tile-wide cleaning

While the existing test cases thoroughly cover solution transfers and capacity scenarios, consider adding test cases that specifically validate the new "clean entire tile" functionality mentioned in the PR objectives. These could include:

  • Multiple absorbents on the same tile
  • Position-dependent cleaning scenarios
  • Edge cases for tile boundaries

Let's check if there are any related test files that might need similar updates:

Content.Shared/CCVar/CCVars.cs (1)

1680-1680: LGTM: Emergency shuttle auto-call disabled

The change from 120 to 0 minutes effectively disables the automatic emergency shuttle call feature, which aligns with the PR objective of removing automatic shift completion.

Run this script to check for any other related emergency shuttle configurations that might need adjustment:

✅ Verification successful

Based on the search results, I can now verify the implications of changing EmergencyShuttleAutoCallTime from 120 to 0:

Automatic shuttle call feature disabled as intended

The change to set EmergencyShuttleAutoCallTime to 0 correctly implements the PR's objective of removing automatic shift completion. This CVar specifically controls the automatic emergency shuttle call timer, and setting it to 0 effectively disables the feature.

The codebase shows this is a clean change that:

  • Only affects the auto-call timing mechanism
  • Doesn't break any dependent systems, as manual shuttle calls via communications console and admin commands remain functional
  • Aligns with the game's architecture where EmergencyShuttleSystem checks this value to determine if/when to auto-call the shuttle
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any other emergency shuttle related configurations
# that might need to be adjusted for consistency

rg -A 2 "EmergencyShuttle|emergency.*shuttle|shuttle.*emergency"

Length of output: 99643

Comment on lines +181 to +184
footstepSoundCollection:
collection: FootstepWater
params:
volume: 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the search results, I can now make a determination about the footstep sound volume. The search revealed various volume settings across different entities in the game, particularly:

  1. Water tiles have volume: 8
  2. Most ambient/environmental sounds range from -8 to -14
  3. Most footstep-related sounds in NPCs/mobs range from -6 to -2
  4. Regular interaction sounds (like tools dropping) are around -2 to -4

The current footstep sound volume of 3 appears to be too high compared to other similar sound effects in the game, especially considering that footsteps are frequent ambient sounds that should be subtle.

Reduce footstep sound volume

The footstep sound volume (3) is too high compared to other ambient and footstep sounds in the game. Consider reducing it to a negative value between -2 and -4 to match other footstep volumes and avoid overwhelming other game sounds.

🔗 Analysis chain

Verify footstep sound volume

The footstep sound volume is set to 3, which might be too loud relative to other sound effects in the game.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other FootstepModifier components to compare volume settings
rg -A 5 "FootstepModifier:|volume:" Resources/Prototypes/Entities/

Length of output: 101147

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs (1)

299-331: Consider extracting puddle processing logic.

The puddle processing loop is quite long and handles multiple responsibilities. Consider extracting it into a separate method for better maintainability.

-foreach (var (entity, component) in targets)
-{
-    if (!_solutionContainerSystem.ResolveSolution(entity, component.SolutionName, ref component.Solution, out var puddleSolution) || puddleSolution.Volume <= 0)
-        continue;
-    // ... rest of the processing logic
-}
+private bool ProcessPuddle(
+    EntityUid entity,
+    PuddleComponent component,
+    EntityUid user,
+    EntityUid used,
+    Entity<SolutionComponent> absorberSoln,
+    AbsorbentComponent absorber,
+    TileRef tileRef)
+{
+    if (!_solutionContainerSystem.ResolveSolution(entity, component.SolutionName, ref component.Solution, out var puddleSolution) || puddleSolution.Volume <= 0)
+        return false;
+    // ... rest of the processing logic
+    return true;
+}

+foreach (var (entity, component) in targets)
+{
+    playSound |= ProcessPuddle(entity, component, user, used, absorberSoln, absorber, tileRef);
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6b85573 and dc9cb79.

📒 Files selected for processing (2)
  • Content.IntegrationTests/Tests/Fluids/AbsorbentTest.cs (3 hunks)
  • Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Content.IntegrationTests/Tests/Fluids/AbsorbentTest.cs
🧰 Additional context used
📓 Learnings (1)
Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs (1)
Learnt from: Spatison
PR: WWhiteDreamProject/wwdpublic#131
File: Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs:102-103
Timestamp: 2024-11-29T23:58:34.392Z
Learning: In the C# file `Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs` within the Robust framework, in the `OnInteractNoHand` method of the `AbsorbentSystem` class, it's appropriate to use `uid` for both `user` and `used` parameters in `Mop()` since the object uses itself.
🔇 Additional comments (6)
Content.Server/Fluids/EntitySystems/AbsorbentSystem.cs (6)

37-40: LGTM! Dependencies and constant are well-defined.

The new dependencies and AbsorptionRange constant are properly defined and will support the enhanced area-based absorption functionality.


102-103: Using uid for both user and used parameters is correct.

In the OnInteractNoHand method, using uid for both parameters is appropriate since the object uses itself.


Line range hint 115-132: LGTM! Method signature improvement enhances flexibility.

The updated signature with optional target and coordinates parameters provides better control over the mopping interaction, supporting both targeted and area-based cleaning.


285-291: LGTM! Grid-based puddle detection is well implemented.

The implementation correctly uses the map system to find and process puddles within the absorption range. The use of HashSet for targets is efficient.


334-338: LGTM! Sound effect handling is improved.

The sound effect is now correctly played only when actual absorption occurs, preventing unnecessary sound effects.


138-138: Verify the need for public visibility.

The method visibility has been changed from private to public. While this doesn't introduce security concerns, please verify if this change is intentional and required by other systems.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
Resources/Prototypes/Entities/Objects/Weapons/Melee/pickaxe.yml (2)

Heavy attack parameters are inconsistent with other melee weapons

The heavy attack modifiers for the pickaxe significantly deviate from the established pattern in other melee weapons:

  • Other weapons typically have heavyDamageBaseModifier ≥ 1.0 (20% more damage), while pickaxe has 0.5 (50% less damage)
  • Most weapons have heavyStaminaCost between 5-20, while pickaxe has 0
  • The heavyRateModifier of 0.8 is within normal range (0.5-0.9)

This makes heavy attacks strictly inferior to normal attacks for the pickaxe, contrary to the game's design where heavy attacks trade stamina and speed for increased damage.

🔗 Analysis chain

Line range hint 28-31: Review heavy attack parameters for game balance

The new heavy attack modifiers seem counterintuitive from a gameplay perspective:

  • Heavy attacks are 20% slower but deal 50% less damage
  • No stamina cost removes the trade-off mechanism for heavy attacks

This might make heavy attacks strictly worse than normal attacks, potentially making the feature redundant.

Let's check if this is consistent with other melee weapons:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for heavy attack parameters in other melee weapons
rg "heavyDamageBaseModifier|heavyRateModifier|heavyStaminaCost" "Resources/Prototypes/Entities/Objects/Weapons/Melee/"

Length of output: 3337


Based on the search results, I can now generate a final response. The changes to the pickaxe and mining drill appear to be combat-related adjustments without any corresponding bug reports or mining-related issues in the codebase. The modifications focus on combat mechanics (heavyStaminaCost, heavyDamageBaseModifier, etc.) while maintaining the same structural damage values that affect mining functionality.

Combat balance changes need justification

These modifications appear to be combat balance adjustments rather than bug fixes:

  • Added heavy attack modifiers to pickaxe
  • Modified mining drill's combat stats
  • No changes to mining-related damage values (Structural damage)

The changes should either:

  1. Be moved to a separate balance-focused PR
  2. Include documentation of the specific bugs these combat changes are addressing
🔗 Analysis chain

Line range hint 1-74: Verify alignment with PR objectives

These changes appear to be balance adjustments rather than bug fixes. The PR description doesn't mention any combat-related issues or mining tool balance changes that these modifications would address.

Please clarify:

  1. What bugs are these changes fixing?
  2. Were there reported issues with mining tool combat mechanics?
  3. Has this been tested for mining functionality impact?

Let's check for related bug reports or discussions:


Note: If the GitHub CLI commands fail (as they might in the sandbox), let's try a different approach:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for related bug reports or discussions in recent commits
git log --grep="mining\|pickaxe\|drill" --since="1 month ago" --pretty=format:"%h %s"

Length of output: 0


Script:

#!/bin/bash
# Let's search for related bug reports or discussions in issue comments and PR descriptions
gh issue list --search "mining drill pickaxe combat" --state all --limit 50 --json title,body,state

# Also search for recent changes to these files
git log -p -- "Resources/Prototypes/Entities/Objects/Weapons/Melee/pickaxe.yml" --since="3 months ago"

# Let's also check for related configuration files that might define combat mechanics
fd "\.yml$" | grep -i "combat\|weapon\|mining"

Length of output: 12564


Script:

#!/bin/bash
# Search for related combat/mining configuration patterns in the codebase
rg -l "MeleeWeapon|heavyStaminaCost|heavyDamageBaseModifier" --type yaml

# Look for other mining tools to compare configurations
rg "parent: BaseItem.*Pickaxe|MiningDrill" --type yaml -A 5

Length of output: 13095

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dc9cb79 and 365648f.

⛔ Files ignored due to path filters (1)
  • Resources/Audio/Announcers/RuOld/events/aliens.ogg is excluded by !**/*.ogg
📒 Files selected for processing (1)
  • Resources/Prototypes/Entities/Objects/Weapons/Melee/pickaxe.yml (1 hunks)

Comment on lines +69 to +74
Blunt: 2 # WD EDIT
Slash: 3
Structural: 12
bluntStaminaDamageFactor: 4.0
heavyRateModifier: 1
heavyRangeModifier: 2
heavyDamageBaseModifier: 1
heavyStaminaCost: 10
maxTargets: 3
angle: 20
attackRate: 3.5 # WD EDIT
heavyStaminaCost: 0 # WD EDIT
angle: 0 # WD EDIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Review mining drill balance changes

The combination of changes to the mining drill raises several concerns:

  1. The 7x increase in attack rate (3.5 vs 0.5) combined with autoAttack: true and no stamina cost could make it overpowered
  2. Zero attack angle with auto-attack might make it too precise compared to other tools
  3. The significant reduction in blunt damage might be intended to balance the increased attack rate, but the DPS increase is still substantial

Consider:

  • Adding a stamina cost to prevent continuous use
  • Keeping a small attack angle for more realistic drilling behavior
  • Further adjusting the attack rate or damage values to maintain balanced DPS

Let's verify these values against similar tools:

#!/bin/bash
# Search for attack rates and angles in other auto-attack weapons
rg "autoAttack: true" -A 10 "Resources/Prototypes/Entities/Objects/"

@Remuchi Remuchi merged commit 1cd5b0a into master Dec 1, 2024
10 checks passed
@Remuchi Remuchi deleted the bugfixs branch December 1, 2024 13:41
Kutosss pushed a commit to Kutosss/kutos that referenced this pull request Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants